Repository navigation
fix(platform)!: refuse audited requests on a stalled stdout audit stream - #2019
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d1188590c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aaaeda06d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A stdout audit destination whose reader stops reading held every audited request for as long as the reader stayed away: each append waited on the stream lock behind the stuck write, nothing bounded the wait, readiness stayed true, and dropping the writer joined the stuck thread. One thread now owns the stream and writes the lines handed to it in order. An append waits at most 5 seconds for its line. A destination holding one line past that deadline is stalled: the waiting request is refused with the added AuditUnavailableReason::Stalled, later requests are refused at once, and ready() is false until the destination takes the line, after which the writer accepts entries again with no restart. A failed write still stops the writer until restart. The queue behind the line being written is bounded, and a full queue refuses instead of growing. Dropping the writer waits one deadline for the thread, then leaves it. The start and the end of a stall are each logged once, with no entry content. The line the destination is holding when its caller's deadline passes is still written when the destination takes it; the crate README records that caveat with the review notes. BREAKING: AuditUnavailableReason gains the Stalled variant, and a stream append waits on the Tokio timer, so the runtime that appends needs its time driver. Without one the append is refused before its line is queued. The schedulingctl test that appended on a runtime without one enables it. Refs #1566 Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A stream destination that held only the unfinished response of a dropped request past the write deadline logged the end of the stall and no start, because the start is reported by the append or readiness check that first sees the stall and nothing looked at the stream. The write that ends such a stall now reports its start first, so the operator reads both records. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…k saw An append's wait starts before its line is handed to the stream thread, so when that wait runs out the write in progress can be a little younger than the deadline and the refused caller reports no stall. The drop test asserted the start report straight after the refusal and failed whenever the thread picked the line up late. It now waits for readiness to turn false first, and the comment on the refusal states what the code holds. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The review record read as an end-to-end result. It now says that the writer's reports reach an operator only where the product's log filter admits the writer's target, names the products on each side, and says that a product logging to the standard output its audit stream writes can block in its own log write, which the deadline does not bound. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A full queue refuses an awaited line with no stall report, but an unfinished response entry it refuses is still reported as not accepted. The review record said a full queue refuses lines without a report, which read as covering both. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
aaaeda0 to
34182ef
Compare
Summary
A stdout audit destination whose reader stops reading held every audited
request for as long as the reader stayed away. This change bounds that wait in
registry-platform-audit(owning area:crates/, with the platform changelogand security principles under
products/platform).line past that deadline is stalled: the waiting request is refused with the
added
AuditUnavailableReason::Stalled, later requests are refused at once,and
ready()is false until the destination takes the line. The writer thenaccepts entries again with no restart.
queue refuses instead of growing.
The deadline is a constant, not a configuration key. A shared audit
configuration block across products is tracked in #2006.
Evidence
cargo test --locked -p registry-platform-audit: 146 passed.The tests that cover the change are
a_stalled_stream_refuses_the_append_at_the_deadline_and_later_ones_at_once,a_write_error_still_stops_the_stream_after_its_sink_recovers,a_stalled_stream_leaves_the_runtime_to_other_tasks,a_full_stream_queue_refuses_a_line_instead_of_growing,a_runtime_without_a_timer_refuses_the_append_before_queueing_its_line,dropping_a_request_never_waits_on_a_stalled_stream,dropping_the_writer_waits_at_most_one_deadline_for_a_blocked_stream,readiness_follows_a_stall_and_returns_once_the_stream_drains, anda_stall_nothing_looked_at_reports_its_start_before_its_end. The lastone failed before its fix (a stall no append or readiness check saw
reported its end with no start) and passes after.
registry-schedulingunit tests (118 passed):a_stalled_destination_is_not_ready_until_it_drainsproves a product'sreadiness follows the stall and its end, with the real 5-second deadline.
cargo fmt --checkandcargo clippy --locked --workspace --all-targets -- -D warnings: clean.cargo test --locked --workspace --no-fail-faston macOS arm64: all targetspass except three, with 7 failing tests that do not exercise this change:
registry-casework --test install_script(5 tests, the installer scriptcannot read its fixture release binary),
registry-evidencectl --test access(1) and
--test jwk_from_pem(1), both stopped bydyldfailing to loadlibaws_lc_fipsin a spawnedevidencectl. The same 7 testsfail the same way on this machine with the crates at
origin/main.main.After the rebase,
cargo fmt --check, clippy with-D warnings, andcargo test --lockedwere repeated forregistry-platform-audit,registry-schedulingandregistry-schedulingctl: clean, 332 passed,0 failed.
driver, except the one test here that proves the refusal without one.
cargo deny check, the PostgreSQL suites that need a disposabledatabase, and the docs site checks (no docs site file changes).
Notes
Security-sensitive: audit integrity. The review notes follow in their own
section and are kept in full in
crates/registry-platform-audit/README.md.AuditUnavailableReasongains theStalledvariant, and astream append waits on the Tokio timer, so the runtime that appends needs its
time driver. Without one the append is refused before its line is queued.
when its caller's deadline passes is still written when the destination
takes it, so the audit stream can carry an entry for a request that was
refused. A refused entry does not undo work a product already committed.
not bytes; a full queue, or a destination slow enough that a line waits
out its deadline in the queue, refuses appends but does not turn
ready()false;the start of a stall is logged by the first append or readiness check
that sees it, and otherwise just before its end.
docs/siteis carried by docs(platform): tell operators how a stalled stdout audit stream behaves #2027, which isstacked on this branch.
Refs #1566
Security review notes
Threat. An audited request waits for its audit entry before protected I/O or disclosure. A
stdoutaudit destination whose reader stops taking lines held that wait with no bound, so one stuck collector could hold every audited request and leave readiness reporting healthy.Enforcement point.
registry-platform-audit,LineStreaminsrc/writer.rs. Oneaudit-streamthread owns the sink. An append hands its line to a bounded queue (STREAM_QUEUE_LINES, 4096) without waiting for room and waits at mostSTREAM_WRITE_DEADLINE(5 seconds, fixed, no configuration key) on the Tokio timer. A destination that holds one line past the deadline is stalled: the waiting append and every later one returnAuditUnavailableReason::Stalled, andAuditWriter::ready()is false until the destination takes the line. The writer then accepts entries again with no restart.What still refuses. A failed stream write stops the writer until restart (
WriteFailed). A full queue refuses the line at once (Stalled). A runtime with no time driver refuses the append before its line is queued (Stopped). A stream whose thread has ended refuses (Stopped). In each case the caller gets no accepted entry, so the product blocks protected I/O or disclosure as it does for any failed entry.Proven and not proven.
Proven by tests in
registry-platform-audit:a_stalled_stream_refuses_the_append_at_the_deadline_and_later_ones_at_oncereadiness_follows_a_stall_and_returns_once_the_stream_drainsa_write_error_still_stops_the_stream_after_its_sink_recoversa_full_stream_queue_refuses_a_line_instead_of_growinga_runtime_without_a_timer_refuses_the_append_before_queueing_its_linedropping_the_writer_waits_at_most_one_deadline_for_a_blocked_streama_stall_nothing_looked_at_reports_its_start_before_its_endProven in
registry-scheduling:a_stalled_destination_is_not_ready_until_it_drains.Not proven:
ready()false and without a stall report.ready()still true and no stall report. No test holds this case; it is read fromLineStream::appendandStreamState::run.registry_platform_audit::writer. By reading, Evidence and Scheduling admit it, and Base Registry Engine, Casework, Messaging,breg-mcp, andbreg-reviewdo not.DCO
Signed-off-bytrailer.